I'm not sure about the submodules. What about using FetchContent? Isn't that supposed to be the new way to do this?

+1 for FetchContent (this is how we integrate abseil-cpp, protobuf and pybind11 in google/or-tools)
see: https://github.com/google/or-tools/blob/9021ffbb9518201f6cc89e50bb25551913edff9d/cmake/dependencies/CMakeLists.txt#L59-L106
and: https://github.com/google/or-tools/blob/9021ffbb9518201f6cc89e50bb25551913edff9d/cmake/dependencies/CMakeLists.txt#L147-L167

also you should provide a find_package() way aka using a system wide install of the dependencies like any Distro/Homebrew packages (also supported in google/or-tools)
see:
https://github.com/google/or-tools/blob/9021ffbb9518201f6cc89e50bb25551913edff9d/cmake/deps.cmake#L19-L21
https://github.com/google/or-tools/blob/9021ffbb9518201f6cc89e50bb25551913edff9d/cmake/deps.cmake#L44-L49
https://github.com/google/or-tools/blob/9021ffbb9518201f6cc89e50bb25551913edff9d/cmake/deps.cmake#L122-L127

note: up to know we were using SWIG to wrap in our CMake based build, now we are slowly migrating to pybind11 and also add these new pybind11 wrapper to our bazel based build also.
note: we target macos, windows vs2019/2022, and few distro:
alpine-edge,
centos-8, centos-7,
debian-11, debian-10,
fedora-35, fedora-34, fedora-33
opensuse-leap
ubuntu-22.04 ubuntu-20.04 ubuntu-18.04

So I should work on it for Q2 2022 (next week hopefully)

Had cause to revisit this and have updated using FetchContent (loosely based on google/or-tools as per suggestion above). Happy to restructure as needed.

Thanks, LGTM, with my extremely superficial cmake background.

I could now do the manual step to get this merged ("submitted" is our actual term) internally, as I outlined before (#60 (comment)), which will then trigger the automatic export. But before I do so, one quick question:

It's not great that we don't have any testing. Are you interested in adding a minimal GHA workflow (e.g. just one platform) to exercise the new cmake files? In this PR, or separately?

Sure, I’ll put together a workflow for Ubuntu latest that does a build and runs the tests.

I have one question about which versions of the dependencies to pin to in FetchContent. I have used the versions used in the or-tools project rather the the versions in this projects Bazel files because the latter combination had build issues on macOS. If consistency across build systems is needed then there’s a bit more to do there as well.

Sure, I’ll put together a workflow for Ubuntu latest that does a build and runs the tests.

Awesome, thanks!

I have one question about which versions of the dependencies to pin to in FetchContent. I have used the versions used in the or-tools project rather the the versions in this projects Bazel files because the latter combination had build issues on macOS. If consistency across build systems is needed then there’s a bit more to do there as well.

My feeling: "more to do" can wait (future PR).

@laramiel what do you think?

General purely practical consideration:

• With the current setup it's a bit of grunge work for me to merge internally, therefore handling many small PRs is time consuming,
• but I wouldn't want to pack too much into one PR just for the sake of saving me some grunge work.

Friendly ping @rwgk and @laramiel, we are looking to get this merged for use in our project. Is there anything we can do to move this along?

@mjcarroll - actually I think that's with me to put together a minimal workflow using the cmake build. If you're thinking of using this for gz-msgs and gz-transport bindings I'll get back on to it.

I'll get back on to it.

Nice! (Let me know if simply integrating this PR would help, even though "no testing" means no assurance that it won't break.)

@rwgk if you're ok with merging this as is that would help for sure. I'll raise an issue for the ci workflow which you can assign to me if you like.

@rwgk if you're ok with merging this as is that would help for sure. I'll raise an issue for the ci workflow which you can assign to me if you like.

Is there a reasonably easy way to print out a warning when cmake is run (the initial configure step)?

Something like:

WARNING: CI testing for pybind11_protobuf cmake is currently MISSING! (https://github.com/pybind/pybind11_protobuf/issues/104)

Is there a reasonably easy way to print out a warning when cmake is run (the initial configure step)?

Sure - the following should do it. I'll add it in.

message(WARNING "CI testing for pybind11_protobuf cmake is currently MISSING! (https://github.com/pybind/pybind11_protobuf/issues/104)")

Output is:

 % cmake ..
CMake Warning at CMakeLists.txt:18 (message):
  CI testing for pybind11_protobuf cmake is currently MISSING!
  (https://github.com/pybind/pybind11_protobuf/issues/104)


-- Build abseil-cpp: ON
-- Build protobuf: ON
-- Python: Build pybind11: ON
-- Fetching Abseil-cpp
...

Thanks, looks great! I'll try to pull this in asap.

GHW for cpplint, cppcheck and build on https://github.com/srmainwaring/pybind11_protobuf/tree/feature/cmake-ci. It is a branch off this PR.

I noticed that for the an ubuntu build the dependencies need to be built with `CMAKE_CXX_FLAGS="-fPIC". This can be done either on the command line when calling cmake, or defined in the top-level CMakeLists.txt, which is what I've done for the follow up PR.

If you prefer I can push the CI changes to this branch (and remove the warning).

I tried to establish basic pre-commit checks, including cmake-format: #105

I don't really know what I'm doing but it seems to work as a start. Note that your cmake files have a lot of whitespace changes.

Idea from here:

I could remove your cmake files from #105 again and merge the rest.

Then you could rebase this PR, then run pre-commit run --all-files for the whitespace cleanup.

And yes: pushing the CI changes here would be ideal.

How does that sound?

I could remove your cmake files from #105 again and merge the rest.

I decided to go ahead and do that. #105 will hopefully auto-merge in a few minutes.

@rwgk - could you trigger/approve the GHW pls.

Update: This PR and #108 (auto-force-pushed from Google-internal change list) are exactly identical again.

Merged via #108.

Hi @rwgk - thanks for getting this reviewed and merged. I noticed that my authorship has been stripped from the commits in main. Is there any way of getting that fixed?

Hi @rwgk - thanks for getting this reviewed and merged. I noticed that my authorship has been stripped from the commits in main. Is there any way of getting that fixed?

Sorry for that. I need to find out. Our "copybara" export tool (see #108) is not easy to work with.

FYI

PR #109 rolls back #108 (I don't know why the new PR isn't marked more obviously as a rollback).

Then I'll try a roll forward with a trick to set the original author. It's a bit of an experiments, I hope it'll work out.

Small update: we tried very hard to convince our copybara tool to respect some original author setting, but it stubbornly keep using my name (#110). Will try more tomorrow (hopefully).

Thanks @rwgk - much appreciated.

It worked!

d824311

It turns out, we had such a hard time yesterday because there's a bug in the copybara tool. Luckily that only affects pre-merge testing. In the merging step it propagates the original author information correctly. — Thanks a lot @laramiel for helping me in the troubleshooting!